feat: add Page Break field type for multistep forms#150
Conversation
📝 WalkthroughWalkthroughAdds a Page Break form field type: frontend/back-end type declarations, backend mapping to Frappe "Tab Break" and display-only behavior, adjusted get_options() logic (no heading HTML for Page Break), plus unit and integration tests for validation, persistence, and DocType sync. ChangesPage Break Field Type Support
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
forms_pro/tests/test_page_break.py (1)
8-50: ⚡ Quick winConsider strengthening assertions to verify DocType sync.
The test currently only asserts that
"Page Break"appears inform.fieldsafter reload (line 50). While this confirms form-level persistence, it doesn't verify that the Page Break field synced correctly to the linked DocType with the expectedHTMLfieldtype and<hr>options—unliketest_page_break_syncs_as_html_to_linked_doctypewhich does verify the sync contract.Consider adding assertions similar to lines 68-72 in the second test to confirm the full persistence chain in this multi-field scenario as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forms_pro/tests/test_page_break.py` around lines 8 - 50, The test test_form_with_page_break_saves only checks that "Page Break" exists in form.fields after reload; enhance it to also verify the linked DocType sync by locating the generated DocType for the form (after form.save() / form.reload()) and asserting that the corresponding field was created there with fieldtype "HTML" and options "<hr>" (mirror the assertions in test_page_break_syncs_as_html_to_linked_doctype). Update the test to fetch the linked DocType (using the same lookup used in the other test), find the field by fieldname (e.g., "step_2"), and assert its fieldtype and options match the expected synced values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@forms_pro/tests/test_page_break.py`:
- Around line 8-50: The test test_form_with_page_break_saves only checks that
"Page Break" exists in form.fields after reload; enhance it to also verify the
linked DocType sync by locating the generated DocType for the form (after
form.save() / form.reload()) and asserting that the corresponding field was
created there with fieldtype "HTML" and options "<hr>" (mirror the assertions in
test_page_break_syncs_as_html_to_linked_doctype). Update the test to fetch the
linked DocType (using the same lookup used in the other test), find the field by
fieldname (e.g., "step_2"), and assert its fieldtype and options match the
expected synced values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77184cf1-2c0f-4988-92fa-a547e859caeb
📒 Files selected for processing (5)
forms_pro/api/submission/test_submission_validation.pyforms_pro/forms_pro/doctype/form_field/form_field.jsonforms_pro/forms_pro/doctype/form_field/form_field.pyforms_pro/tests/test_page_break.pyfrontend/src/types/FormsPro/form_field.types.ts
Tab Break is a native Frappe layout field that renders proper tabs in Desk view, replacing the raw HTML <hr> workaround.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forms_pro/forms_pro/doctype/form_field/form_field.py (1)
59-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the auto-generated
DF.Literalunion to includePage Break.The runtime map supports
"Page Break", but the TYPE_CHECKINGfieldtypeliteral list does not. This creates a cross-layer type contract mismatch (editor/type-check errors despite valid runtime value). Please regenerate this block so"Page Break"is present in the union.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forms_pro/forms_pro/doctype/form_field/form_field.py` around lines 59 - 82, The DF.Literal type list for the fieldtype attribute is missing "Page Break", causing a type-check mismatch; update/regenerate the DF.Literal union used by the fieldtype variable (the literal block assigned to fieldtype in form_field.py) to include "Page Break" alongside the other entries so the TYPE_CHECKING literal matches the runtime map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@forms_pro/forms_pro/doctype/form_field/form_field.py`:
- Around line 59-82: The DF.Literal type list for the fieldtype attribute is
missing "Page Break", causing a type-check mismatch; update/regenerate the
DF.Literal union used by the fieldtype variable (the literal block assigned to
fieldtype in form_field.py) to include "Page Break" alongside the other entries
so the TYPE_CHECKING literal matches the runtime map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07154101-4eaa-42aa-be7f-dd4dd3ec8d6b
📒 Files selected for processing (3)
forms_pro/api/submission/test_submission_validation.pyforms_pro/forms_pro/doctype/form_field/form_field.pyforms_pro/tests/test_page_break.py
🚧 Files skipped from review as they are similar to previous changes (1)
- forms_pro/tests/test_page_break.py
1 participant
Summary
Page Breakas a new display-only field type (like Heading 1/2/3)HTMLfieldtype, generates<hr>on linked DocTypeChanges
form_field.json— addedPage Breakto fieldtype optionsform_field.py— mapping, display-only set,get_options()returns<hr>test_submission_validation.py— 6 unit teststest_page_break.py— 2 integration testsform_field.types.ts— auto-generated enum memberTest plan
bench run-tests --app forms_pro)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests